Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

+semver:major Added properties to ArchivingDlgViewModel to facilitate customizing the initial summary #1349

Merged
merged 7 commits into from
Oct 8, 2024

Conversation

tombogle
Copy link
Contributor

@tombogle tombogle commented Oct 4, 2024

Added unit tests and changed test app to illustrate intended usage of the new properties. Improved/added some comments in ArchivingDlgViewModel

Note: I considered removing OverrideDisplayInitialSummary because with this change, I anticipate that it will no longer be needed, but since it is inexpensive to leave in and allows for maximum flexibility, I decided to leave it.


This change is Reviewable

Copy link

github-actions bot commented Oct 4, 2024

LibPalaso Tests

    35 files  ±0      35 suites  ±0   12m 1s ⏱️ +2s
 4 828 tests +4   4 597 ✅ +4  231 💤 ±0  0 ❌ ±0 
11 052 runs  +8  10 524 ✅ +8  528 💤 ±0  0 ❌ ±0 

Results for commit 599884c. ± Comparison against base commit f22556e.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me, I left a couple suggestions, though I'm not sure why this is a semver major change, I don't see any breaking changes, just new features added.

SIL.Archiving.Tests/IMDIArchivingDlgViewModelTests.cs Outdated Show resolved Hide resolved
SIL.Archiving/ArchivingDlgViewModel.cs Outdated Show resolved Hide resolved
@tombogle tombogle changed the title +semver:major Added properties to ArchivingDlgViewModel to facilitate customizing the initial summary +semver:minor Added properties to ArchivingDlgViewModel to facilitate customizing the initial summary Oct 7, 2024
@tombogle tombogle force-pushed the archiving-api-improvements branch from b75a621 to b208966 Compare October 7, 2024 14:13
… customizing the initial summary

Added unit tests and changed test app to illustrate intended usage of the new properties.
Improved/added some comments in ArchivingDlgViewModel
…ach of multiple multiple pre-archiving messages
TEMP: Added some console.log statements to try to see what's happening with TC tests
@tombogle tombogle force-pushed the archiving-api-improvements branch from 776fa1a to 680d783 Compare October 7, 2024 14:18
@tombogle tombogle changed the title +semver:minor Added properties to ArchivingDlgViewModel to facilitate customizing the initial summary +semver:major Added properties to ArchivingDlgViewModel to facilitate customizing the initial summary Oct 7, 2024
@tombogle
Copy link
Contributor Author

tombogle commented Oct 7, 2024

I think it is a technically a major version number change because I renamed a parameter. In most cases, it will recompile with no actual code changes, but if a caller were to use named parameters, then it could break. (Originally, I had intended to remove a method as well, but I decided to leave it.) In any case, this is already going to be included in a release that will cause a bump in the major version number.

@tombogle tombogle merged commit cd24738 into master Oct 8, 2024
3 of 4 checks passed
@tombogle tombogle deleted the archiving-api-improvements branch October 8, 2024 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants